feat(header, footer, tab-bar): add scrollEffect prop#31263
feat(header, footer, tab-bar): add scrollEffect prop#31263OS-susmitabhowmik wants to merge 72 commits into
Conversation
…rrect theme class usage
…orce footer priority
…llEffect in all modes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
package: core
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Hey, great work on this! I highlighted a few issues I found, some of them aren't as important as others, but still
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Hey, I think we're finally getting close to getting this one wrapped up! Just a few more issues I noticed, but we're so close!
2a64a10 to
7756065
Compare
@ShaneK Thanks for all the help on this! I addressed your latest round of comments and also noticed that some tests were using the deprecated |
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Thanks for those changes, we're looking amazing! Just a few remaining things I noticed and I think we're done after this!
| hiddenClass: 'tab-bar-scroll-hidden', | ||
| contentPartnerClass: 'content-tab-bar-hide-scroll-partner', | ||
| contentHiddenClass: 'content-tab-bar-hide-scroll-hidden', | ||
| shouldKeepAriaHidden: () => this.keyboardVisible, |
There was a problem hiding this comment.
| shouldKeepAriaHidden: () => this.keyboardVisible, | |
| shouldKeepAriaHidden: () => this.keyboardVisible && this.el.getAttribute('slot') !== 'top', |
I pulled the branch and hit this one: a slot="top" tab-bar can get left with aria-hidden="true" while it's fully visible. The keep-guard here is this.keyboardVisible, but the keyboard writer above only hides bottom bars (keyboardOpen && slot !== 'top'). So with the keyboard open on a top tab bar, scrolling down hides it (aria-hidden set, correct), then scrolling back up runs setHidden(false), which clears inert and the hidden class but keeps aria-hidden because keyboardVisible is still true. I reproduced it live: after scrolling back up the bar is inert=false, not scroll-hidden, but still aria-hidden="true" until the keyboard closes. Matching the guard to the keyboard writer fixes it.
| // -------------------------------------------------- | ||
|
|
||
| :host(.tab-bar-hide-on-scroll) { | ||
| :host(.tab-bar-scroll-effect-hide) { |
There was a problem hiding this comment.
I think prefers-reduced-motion gets defeated for the ionic theme here. Since this file does @use "./tab-bar.common", common's reduced-motion transition: none block is emitted first, then these ionic rules re-declare transition at equal specificity further down, so they win under reduced motion. I checked with reduced-motion emulated: on md the shown and hidden durations are both 0s, but on ionic the show transition stays 0.2s and the compact hide stays 0.35s (only the full-expand hide gets suppressed), so a reduced-motion user still gets the slide. I think adding a @media (prefers-reduced-motion: reduce) override at the end of this file, or extending the common reduced-motion selector list to cover these ionic selectors, would close it.
There was a problem hiding this comment.
Great catch! I added the prefers-reduced-motion block to the end of the ionic tab bar stylesheet in 62dd0f5
| return false; | ||
| } | ||
|
|
||
| this.scrollEl = await getScrollElement(contentEl); |
There was a problem hiding this comment.
Not a blocker, more of a consistency thought now that the hide path has the nice promise-identity guard. The condense setup here (and setupFadeHeader, plus footer's setupFadeFooter) still attaches its IntersectionObserver and scroll listener after await getScrollElement with no equivalent guard, and for condense activeEffect is only set after the await. So if a re-render lands in that await window, a second setup spawns and the first observer plus listener leak, since the later assignment overwrites the reference and teardown only cleans one. This is pre-existing structure and this PR actually narrows the window, so I don't think it needs to block anything. Worth extending the same guard to condense/fade at some point, or a follow-up card? Up to you.
There was a problem hiding this comment.
I added a guard to the condense and fade setup methods in this commit eba6da6
016e0d6 to
eba6da6
Compare
3 participants
Issue number: resolves internal
What is the current behavior?
ion-headerandion-footerhave acollapseprop for scroll-driven effects (condense,fade), but it only works on the iOS theme.ion-tab-barhas an unreleased hideOnScroll boolean prop that only works on the Ionic theme withexpand="compact".What is the new behavior?
scrollEffectstring enum prop toion-header,ion-footer, andion-tab-bar.scrollEffect="hide"slides the bar out of view when scrolling down and back in when scrolling up.scrollEffect="condense"andscrollEffect="fade"onion-header/ion-footerreplace the existingcollapsevalues and work across all themes.collapseprop onion-headerandion-footerwith a console warning directing developers to usescrollEffect.hideOnScrollprop fromion-tab-bar.ion-tab-baris nested insideion-footer, the footer owns the hide animation to prevent double-animationDoes this introduce a breaking change?
Other information
Relevant Test Pages:
Header
Deprecated
collapsepropscrollEffectpropFooter
Deprecated
collapsepropscrollEffectpropTab Bar